[fix][broker] Correct two race conditions in the tracker code and logic bug in InMemoryDelayedDeliveryTracker that failed with NoSuchElementException#25681
Merged
Conversation
Contributor
Author
chamons
force-pushed
the
broker_race_condition_fix
branch
3 times, most recently
from
39814a6 to
c11c32f
Compare
chamons
changed the title
chamons
force-pushed
the
broker_race_condition_fix
branch
2 times, most recently
from
992ab28 to
0038cf4
Compare
…rMultipleConsumers - apache#25617 DelayedDeliveryTracker is not thread safe, and any access to it must be done holding the object lock. There were five cases I found, four of them I could correct just by adding synchronized to the method declaration. In one of them I used a manual scope since it was only a subset of the method. This is a significant issue, as exceptions here were uncaught and put the broker into an invalid state that prevented some delayed messages from being delivered until restart. The included unit test failed 100% of the time when run locally without the fix.
…Tracker While reviewing the code for more issues to explain the crashes we were still seeing, I noticed that InMemoryDelayedDeliveryTracker contains two major state fields: - delayedMessageMap - delayedMessagesCount These are kept in sync in various methods, but those methods were _not_ correctly synchronized, which meant it was trivial for us to get them out of sync and then crash peeking at the map. Added a unit test that triggers 100% of the time before the fix.
…o InMemoryDelayedDeliveryTracker
When the same ledger/entry messages come into InMemoryDelayedDeliveryTracker, we can desync the
delayedMessagesCount/delayedMessageMap count such that there are less items in the map than the count
If then we pop down to zero (not clear), we can get:
```
java.util.NoSuchElementException
at java.base/java.util.TreeMap.key(TreeMap.java:1637)
at java.base/java.util.TreeMap.firstKey(TreeMap.java:302)
at org.apache.pulsar.broker.delayed.InMemoryDelayedDeliveryTracker.nextDeliveryTime(InMemoryDelayedDeliveryTracker.java:291)
at org.apache.pulsar.broker.delayed.AbstractDelayedDeliveryTracker.updateTimer(AbstractDelayedDeliveryTracker.java:117)
at org.apache.pulsar.broker.delayed.InMemoryDelayedDeliveryTracker.getScheduledMessages(InMemoryDelayedDeliveryTracker.java:247)
at org.apache.pulsar.broker.delayed.InMemoryDeliveryTrackerTest.testAddMultipleMessagesSameWindow(InMemoryDeliveryTrackerTest.java:328)
```
Instead, we track if the bitmap doesn't already contain the item, and only increment delayedMessagesCount if not.
chamons
force-pushed
the
broker_race_condition_fix
branch
from
0038cf4 to
66df8a9
Compare
lhotari
reviewed
May 8, 2026
lhotari
reviewed
May 8, 2026
…tipleConsumersClassic with test
…DeliveryTracker" This reverts commit 522604d.
Contributor
Author
|
I have resolved all current PR review comments and am ready for a re-review. |
Contributor
Author
|
Thanks @lhotari - Do we have any idea when this fix might make it into a public release? Could it make it into a 4.x branch? We'd like to make some plans, and knowing what timetable to expect would be really helpful. |
Contributor
|
Yes, it's a bug fix and it will be backported into 4.0.x (prob 4.0.11) release |
Member
|
@chamons I have cherry-picked in this to branch-4.0, branch-4.1 and branch-4.2 . Since the implementations in each branch is slightly different, the change has been backported. Please note that 4.1.x is no longer supported, so the release won't be made on that branch although the fix is present on the branch. It's recommended to upgrade to 4.2.2 once it comes out within about a month. In the mean time, you can build your own release. The Pulsar release is performed using these steps: https://pulsar.apache.org/contribute/release-process/#build-release-artifacts . The "Build release artifacts" and "Stage docker images" are the only required steps in this case. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #25617
Motivation
While running a production system under load, we found a traffic pattern that could cause uncaught exceptions in the brokers of our pulsar cluster. If you have a large spike of messages with a matching deliver_at_time, it is possible to trigger java.util.NoSuchElementException and other exceptions:
This is a significant issue, as exceptions here were uncaught and put the broker into an invalid state that prevented some delayed messages from being delivered until restart.
Using our external repro as a guide, we were able to isolate two race conditions and a logic bug that triggered exceptions that matched these.
With these three fixes, we were able to run our sample case for 30 minutes straight without exceptions (before hand it would trigger within 3-5 minutes).
Modifications
Posting each commit's description here for simplicity:
Verifying this change
I have a clean CI here
The included unit tests fail 100% of the time when run locally without the fix, and we were able to run https://github.com/chamons/pulsar-scheduled-exception-repro without issues with the fixes included.
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes